first step to fix dvd-dev/hilo#486 by making room to a second websock… #505
Conversation
…ndling challenge events.
![gitpack-ai[bot]](https://avatars.githubusercontent.com/in/1004046?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review for this PR
The changes introduce handling for multiple websockets, which is an ambitious and complex feature. While the code attempts to manage connections and reconnections efficiently, there is an increase in complexity due to the use of lists to track multiple tasks. Redundancies in methods and potential maintainability issues are worth addressing for improved robustness.
Positives:
- The handling of websockets seems more robust, aiming to support multiple connections which is a forward-looking enhancement.
- The use of detailed logging will help in debugging and monitoring websocket status, which is a good practice.
Areas of Improvement:
- Consolidate similar methods to reduce redundancy and improve maintainability.
- Enhance comments and documentation around websocket management logic, specifically the reasoning behind list usage for managing tasks.
| self._websocket_reconnect_tasks: list[asyncio.Task | None] = [None, None] | ||
| self._update_task: list[asyncio.Task | None] = [None, None] |
There was a problem hiding this comment.
Switching from a single asyncio.Task to a list for both _websocket_reconnect_tasks and _update_task introduces complexity, which could require additional management logic to handle these lists properly and ensure synchronization. Consider commenting the rationale behind using lists here or ensuring robust handling to avoid potential issues.
custom_components/hilo/__init__.py
Outdated
| async def request_status_update_challenge(self) -> None: | ||
| await self._api.websocket2.send_status() |
There was a problem hiding this comment.
The addition of request_status_update_challenge seems redundant given its similarity to request_status_update. Consider refactoring these methods to reduce code duplication, which will improve maintainability in case of future changes.
|
Will depend on dvd-dev/python-hilo#224 |
Clarifié fonction des 2 websockets avec leur noms
Ça permet les deux connexions maintenant, sans erreur
There was a problem hiding this comment.
Code Review for this PR
The refactoring effort to split websockets into device-specific and challenge-specific areas is a sensible approach to managing complexity and decoupling code. The overall structure and flow have been maintained well, while additional subscription functions have been appropriately modularized. However, careful consideration should be given to maintainability and robustness, especially around areas like task management and failure handling.
Positives:
- The addition of new callback methods improves code modularity, making it easier to extend or maintain.
- The separation of websocket responsibilities demonstrates a good practice for handling multiple connection needs.
Areas of Improvement:
- Consider using a more descriptive data structure instead of list indexing for managing multiple tasks associated with websockets.
- Enhance error handling, particularly around token refresh operations, to add robustness and mitigate potential failure points.
| self._websocket_reconnect_tasks: list[asyncio.Task | None] = [None, None] | ||
| self._update_task: list[asyncio.Task | None] = [None, None] |
There was a problem hiding this comment.
The conversion from a single asyncio.Task to a list does increase flexibility, but ideally, you should use a more descriptive data structure (like a dictionary) here if the tasks have different responsibilities. Using indexes may lead to maintenance challenges.
| for inv_id, inv_cb in self.invocations.items(): | ||
| await inv_cb(inv_id) |
There was a problem hiding this comment.
Iterating over self.invocations in this manner could be resource-intensive depending on the size and nature of the invocations. Consider the potential latency or blocking behavior of each callback invoked.
| self.start_websocket_loop(self._api.websocket_devices, 0) | ||
| ) | ||
| self._websocket_reconnect_tasks[1] = asyncio.create_task( | ||
| self.start_websocket_loop(self._api.websocket_challenges, 1) |
There was a problem hiding this comment.
Refreshing the websocket token might require additional error handling if it fails. Consider wrapping this with a try-except block to prevent cascading errors.
…et handling challenge events.